Add OpenShift service-ca support for authenticated metrics endpoints #3677
Conversation
|
Is there an OPRUN ticket associated with this? |
| - apiGroups: | ||
| - authentication.k8s.io | ||
| resources: | ||
| - tokenreviews | ||
| verbs: | ||
| - create | ||
| - apiGroups: | ||
| - authorization.k8s.io | ||
| resources: | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
Why are these additions needed?
There was a problem hiding this comment.
This is not needed for this PR, but I noticed that we're doing apigroups/resources : * access for a lot of verbs. Ideally we'd reduce scope, starting with this change that explicitly lists the api groups and resources.
| kind: Service | ||
| metadata: | ||
| name: olm-operator-metrics | ||
| name: {{ if .Values.serviceCa.enabled }}{{ .Values.serviceCa.olmOperator.serviceName }}{{ else }}olm-operator-metrics{{ end }} |
There was a problem hiding this comment.
What if .Values.serviceCa.olmOperator.serviceName is blank? Or is that something you'll ensure downstream?
Perhaps the serviceName itself should be in the values.yaml file with a default of olm-operator-metrics?
There was a problem hiding this comment.
Kinda like what you do for service-cert-secret-name below?
|
My main comments are:
|
Adds support for OpenShift service-ca-operator alongside existing cert-manager support for authenticated metrics endpoints. This enables the metrics authentication feature to work in both upstream Kubernetes (with cert-manager) and OpenShift (with service-ca) environments. - Add explicit RBAC permissions for `tokenreviews` and `subjectaccessreviews` required by authentication filters - Add `serviceCa` configuration section to values.yaml with configurable secret/service names - Update deployment templates to support both `certManager` and `serviceCa` modes conditionally - Update service templates to conditionally add service-ca annotations when enabled - Maintain backward compatibility with existing `certManager` and `monitoring` configurations
anik120
force-pushed
the
metrics-auth-openshift-compatibility
branch
from
d8fd680 to
8f9d4d2
Compare
| - apiGroups: | ||
| - authentication.k8s.io | ||
| resources: | ||
| - tokenreviews | ||
| verbs: | ||
| - create | ||
| - apiGroups: | ||
| - authorization.k8s.io | ||
| resources: | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
This is not needed for this PR, but I noticed that we're doing apigroups/resources : * access for a lot of verbs. Ideally we'd reduce scope, starting with this change that explicitly lists the api groups and resources.
| kind: Service | ||
| metadata: | ||
| name: olm-operator-metrics | ||
| name: {{ if .Values.serviceCa.enabled }}{{ .Values.serviceCa.olmOperator.serviceName }}{{ else }}olm-operator-metrics{{ end }} |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 971d680
into
operator-framework:master
…dpoints (operator-framework#3677)" This reverts commit 971d680.
2 participants
Description of the change:
Adds support for OpenShift service-ca-operator alongside existing cert-manager support for authenticated metrics endpoints. This enables the metrics authentication feature to work in both upstream Kubernetes (with cert-manager) and OpenShift (with service-ca) environments.
Changes:
tokenreviewsandsubjectaccessreviewsrequired by authentication filtersserviceCaconfiguration section to values.yaml with configurable secret/service namescertManagerandserviceCamodes conditionallycertManagerandmonitoringconfigurationsMotivation for the change:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue